Format repo and enforce formatting in CI #935
Conversation
bkhouri
requested review from
aciidgh,
jakepetroules,
jonathanpenn,
kulpreetchilana,
matthewseaman,
mhrawdon,
neonichu and
owenv
as code owners
bkhouri
changed the title
|
I created swiftlang/swift-format#1097 to track resolution |
bkhouri
force-pushed
the
t/main/format_repo_and_enforce_formatting
branch
from
2617aa7 to
2f8edab
Compare
|
I'm somewhat hesitant to enforce formatting in CI since support in editors for running swift-format-on-save is spotty and it can be pretty frustrating to need to re-run CI when everything passed except the formatting enforcement |
|
I'm fairly neutral on it, but wouldn't mind formatting being enforced.
It's possible there might be a way to configure swiftlang/github-workflows to make other jobs dependent on the formatting one, which would reduce that concern since you wouldn't waste resources or time waiting for the longer jobs to complete. EDIT: tried this in #937, it's straightforward. |
|
Shall I incorporate #937 into this PR? |
bkhouri
force-pushed
the
t/main/format_repo_and_enforce_formatting
branch
from
2f8edab to
cab13c9
Compare
Update the GitHub workflows to enfore formatting, and include instructions on how to configure `git blame` to ignore the formatting commit.
bkhouri
force-pushed
the
t/main/format_repo_and_enforce_formatting
branch
from
cab13c9 to
46294d1
Compare
|
@swift-ci test |
|
I worked around swiftlang/swift-format#1097 by updating the comment style. I also incorporated #937 in this change, which runs the Soundness and Space format Check builds before the others. |
|
@swift-ci test |
|
I feel pretty strongly that we should not do this. I don't think issues with code formatting is a problem we have (or have ever had), it adds additional overhead to making changes (which are already pretty high-overhead), and it takes away the latitude to make different format choices if it improves readability. We've discussed this several times over the last ten years and have always decided not to do it.
I think if this ever has to be done, even for a fast-to-run CI job, then it's obstructing more than it's helping. Please, let's not do this. |
| "-DCMAKE_Swift_FLAGS='\(sharedSwiftFlags.joined(separator: " "))'" | ||
| ] + cMakeProjectArgs + extraCMakeArgs | ||
| let sharedCMakeArgs = | ||
| [ |
There was a problem hiding this comment.
I don't think we should make this change. The open bracket [ should be on the same line as the =, same as open curly braces for methods, etc.
If we do make this change, then I think the open bracket should be aligned with the l in let. The contents of the array should not be double-indented.
| } | ||
| return result | ||
| } | ||
| func scan<S: Sequence, U>(_ seq: S, _ initial: U, _ combine: (U, S.Element) -> U) -> [U] { |
There was a problem hiding this comment.
I don't think a function like this should be indents inside a Swift compilation condition. Functions should remain flush with other functions in the same notional scope.
I can see making an exception for compilation conditions which are inside procedural logic (such as in host() above) although I'm indifferent to it, but not for functions. If we can't have it both ways, then I think we should leave such indentation up to the author.
| let (key, value) = $0.split("=") | ||
| return (key.trimmingCharacters(in: .whitespaces), value.trimmingCharacters(in: .whitespaces)) | ||
| }) | ||
| properties = Dictionary( |
There was a problem hiding this comment.
I think this sort of formatting decision should be left to the discretion of the author.
| "FamilyIdentifier": .plString("android"), | ||
| "IsDeploymentPlatform": .plString("YES"), | ||
| ]) | ||
| ( |
There was a problem hiding this comment.
I think this sort of formatting decision should be left to the discretion of the author.
In this case I prefer the old version.
| try (max(deploymentTargetRange.min, abi.min_os_version)...deploymentTargetRange.max).map { deploymentTarget in | ||
| var triple = abi.llvm_triple | ||
| triple.vendor = "unknown" // Android NDK uses "none", Swift SDKs use "unknown" | ||
| triple.vendor = "unknown" // Android NDK uses "none", Swift SDKs use "unknown" |
There was a problem hiding this comment.
While I think this change is fine, I also think this is a change that should never cause CI to fail.
If this is the sort of thing we want to enforce, then we should run the formatter monthly or something to fix up any cases that have been introduced. But I also don't think this is really a problem that deserves that amount of effort.
|
I would also vote against doing this for the reasons @mhrawdon outlined. I've even removed a linter from a different project because it was drawing too much attention to stylistic issues that were not hurting anything and distracting from the real issues that the Swift compiler emits. Some issues are purely matters of opinion (like whether a space should be before a colon) and do not hurt readability either way. Other stylistic matters could have a pretty reasonable general rule but then there are always many exceptions to that rule where the exceptions actually produce better readability in that specific instance. Other times the linter takes the absolutist position that a certain language feature should always be avoided even though every tool in Swift has a purpose. |
5 participants
Format the repository